Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add details about underlying message types on input message protocol errors #1252

Closed
wants to merge 2 commits into from

Conversation

tmm1
Copy link

@tmm1 tmm1 commented Sep 28, 2024

in general the protocol errors are all missing a lot of context

…errors

Signed-off-by: Aman Karmani <aman@tmm1.net>
@srikrsna-buf
Copy link
Member

Hey @tmm1 Can you tell us a bit about how often you encountered protocol errors and what the backend was? Because we don't expect users to ever hit these errors.

@tmm1
Copy link
Author

tmm1 commented Oct 4, 2024

We are seeing large numbers of these proto errors and are trying to figure out why. Adding these details here would help.

Comment on lines +154 to +161
{
methodName: spec.method.name,
serviceTypeName: spec.service.typeName,
inputTypeName: spec.method.I.typeName,
outputTypeName: spec.method.O.typeName,
url: context.url,
requestMethod: context.requestMethod,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata is used for errors returned as part of the protocol, fully qualified method name should be sufficient to pin point the rpc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, you are suggesting I remove this as adding metadata would interfere with other parts of the stack?

I can do that. I agree that the rpc method is the main piece of information missing.

@@ -149,8 +149,16 @@ export function transformInvokeImplementation<
const input2 = await inputIt.next();
if (input2.done !== true) {
throw new ConnectError(
"protocol error: received extra input message for unary method",
`protocol error: received extra input message ${spec.method.I.typeName} for unary method ${spec.method.name}`,
Copy link
Member

@srikrsna-buf srikrsna-buf Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully qualified method name is unique and will be sufficient to pinpoint:

Suggested change
`protocol error: received extra input message ${spec.method.I.typeName} for unary method ${spec.method.name}`,
`protocol error: received extra input message for unary method ${spec.service.typeName}.${spec.method.name}`,

@srikrsna-buf
Copy link
Member

Hey @tmm1 Can you tell us what is the client in use?

@tmm1
Copy link
Author

tmm1 commented Oct 9, 2024

we're using connect-es v1.4.0

@tmm1
Copy link
Author

tmm1 commented Oct 10, 2024

Thanks for the guidance. I believe this may be an application specific issue, so I will close for now.

@tmm1 tmm1 closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants